Support TS plugins #327

tomv564 · 2017-08-06T18:53:41Z

An investigation for #326

There are two ways of specifying plugins to be loaded:

plugins declaration in tsconfig.json
--globalPlugins argument to tsserver

There are a few different ways to load the plugin:

locally installed with the project (as sibling to typescript itself)
global npm package
Locations provided via --pluginProbeLocations

This PR contains implementation for loading a locally installed plugin specified in tsconfig.json only, as documented on Erich Gamma's new vscode plugin at https://github.com/Microsoft/vscode-ts-tslint.

Missing:

Review what parts of PluginCreateInfo need to be wired up for other plugins
Support for globalPlugins and pluginProbeLocations
A good way to disable loading plugins altogether for sourcegraph
Tests

codecov · 2017-08-06T18:58:52Z

Codecov Report

Merging #327 into master will decrease coverage by 0.09%.
The diff coverage is 69.73%.

@@            Coverage Diff            @@
##           master     #327     +/-   ##
=========================================
- Coverage    74.8%   74.71%   -0.1%     
=========================================
  Files          14       15      +1     
  Lines        1691     1760     +69     
  Branches      317      331     +14     
=========================================
+ Hits         1265     1315     +50     
- Misses        280      294     +14     
- Partials      146      151      +5

Impacted Files	Coverage Δ
src/match-files.ts	`66.17% <100%> (+0.16%)`	⬆️
src/diagnostics.ts	`71.42% <100%> (+2.19%)`	⬆️
src/typescript-service.ts	`73.06% <100%> (+0.49%)`	⬆️
src/project-manager.ts	`81.95% <38.46%> (-1.81%)`	⬇️
src/plugins.ts	`74.57% <74.57%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c7cd41...42ce6e8. Read the comment docs.

felixfbecker · 2017-08-07T16:14:09Z

Could you add doc comments to all the symbols you added? Makes it easier to review

felixfbecker · 2017-08-07T23:01:08Z

src/project-manager.ts

+		// TODO: add peer node_modules to source path.
+
+		// TODO: determine how to expose this setting.
+		// VS Code starts tsserver with --allowLocalPluginLoads by default: https://github.com/Microsoft/TypeScript/pull/15924


I think this would be good to have a setting for through didChangeWorkspaceConfiguration, but it must be off by default or it would allow remote code execution.

tomv564 · 2017-09-10T12:06:57Z

Plugin support:
The angular and graphql plugins should get all they need from the PluginCreateInfo we provide (languageServer, languageServerHost and project.projectService.logger)

Plugin configuration:
I chose to read settings from initializationOptions as supporting didWorkspaceConfigChange would mean recreating the ProjectConfigurations - I believe plugin config would be unlikely to change after startup.

Plugin loading:
Plugins can be installed locally: i.e. project/node_modules/ts-plugin
Plugins can be installed globally, but the server will only pick them up if it is also installed globally.
Plugins can be manually pointed to using pluginProbeLocations.

Security:
If no flags are passed to initializationOptions, no plugin should be loaded unless the user has installed it as a peer package to javascript-typescript-langserver. Is this sufficient?

tomv564 · 2017-09-17T06:33:07Z

@felixfbecker the plugins code was extracted and tests have been added, care to have a look?

felixfbecker · 2017-09-20T21:31:49Z

src/project-manager.ts

+	 * @param pluginModuleFactory function to create the module
+	 * @param configEntry extra settings from tsconfig to pass to the plugin module
+	 */
+	private enableProxy(pluginModuleFactory: PluginModuleFactory, configEntry: ts.PluginImport) {


can this be called multiple times to wrap with multiple plugins?

Yes, every time enableProxy is called, the current this.service is wrapped, so the desired result is plugin2Wrapper(plugin1Wrapper(tsService))

A name like wrapService would make it more clearer to me then, enable always sounds like an idempotent function (like setting a boolean to true)

felixfbecker · 2017-09-20T21:32:00Z

src/project-manager.ts

@@ -852,10 +864,39 @@ export class ProjectConfiguration {
 			this.logger
 		);
 		this.service = ts.createLanguageService(this.host, this.documentRegistry);
+		const pluginLoader = new PluginLoader(this.rootFilePath, this.fs, this.initializationOptions, this.logger);
+		pluginLoader.loadPlugins(options, this.enableProxy.bind(this));


please use an arrow function, bind is not type safe

felixfbecker · 2017-09-20T21:32:16Z

src/project-manager.ts

+			const pluginModule = pluginModuleFactory({ typescript: ts });
+			this.service = pluginModule.create(info);
+		} catch (e) {
+			this.logger.info(`Plugin activation failed: ${e}`);


this should be an error

felixfbecker · 2017-09-20T21:32:48Z

src/plugins.ts

+import { combinePaths } from './match-files';
+import { InitializationOptions } from './request-type';
+
+// definitions from from TypeScript server/project.ts


could you add a full link to GitHub (with revision)

felixfbecker · 2017-09-20T21:32:53Z

Plugin configuration:
I chose to read settings from initializationOptions as supporting didWorkspaceConfigChange would mean recreating the ProjectConfigurations - I believe plugin config would be unlikely to change after startup.

initializationOptions is not specced to contain the settings, it is typed as any. You should use didWorkspaceConfigChange, but you can filter it to only recreate ProjectConfigurations

felixfbecker · 2017-09-27T02:26:41Z

src/plugins.ts

+	public loadPlugins(options: ts.CompilerOptions, applyProxy: EnableProxyFunc) {
+		// Search our peer node_modules, then any globally-specified probe paths
+		// ../../.. to walk from X/node_modules/javascript-typescript-langserver/lib/project-manager.js to X/node_modules/
+		const searchPaths = [combinePaths(__filename, '../../..'), ...this.pluginProbeLocations];


what is the difference between combinePaths and path.resolve()?

Code is copied from TS project, wanted to keep it as close to original as possible (and we had the function already)

Okay, that wasn't clear to me. Maybe change the wording "based on" at the top of the file to "copied from"?

felixfbecker · 2017-09-27T02:26:52Z

src/plugins.ts

+				}
+
+				// Provide global: true so plugins can detect why they can't find their config
+				this.enablePlugin({ name: globalPluginName, global: true } as ts.PluginImport, searchPaths, applyProxy);


why is global not in the typings?

Code is copied from TS project - fix should be made there

felixfbecker · 2017-09-27T02:27:19Z

src/plugins.ts

+	 */
+	private enablePlugin(pluginConfigEntry: ts.PluginImport, searchPaths: string[], enableProxy: EnableProxyFunc) {
+		for (const searchPath of searchPaths) {
+			const resolvedModule =  this.resolveModule(pluginConfigEntry.name, searchPath) as PluginModuleFactory;


why is this cast needed / why is resolveModule not typed with that return type?

resolveModule was not specific to plugin loading, it also was copied and I tried to keep it close to the original.
If it is exposed through the TS api we could remove it from this plugin.ts.

Is there an open issue for exposing this? If not, could you create one and link it at the top?

felixfbecker · 2017-09-27T02:28:19Z

src/plugins.ts

+	private resolveJavaScriptModule(moduleName: string, initialDir: string, host: ts.ModuleResolutionHost): string {
+		// TODO: this should set jsOnly=true to the internal resolver, but this parameter is not exposed on a public api.
+		const result =
+			ts.nodeModuleNameResolver(moduleName, /* containingFile */ initialDir.replace('\\', '/') + '/package.json', { moduleResolution: ts.ModuleResolutionKind.NodeJs, allowJs: true }, this.resolutionHost, undefined);


please wrap long lines by making one argument per line:

const foo = bar( baz, qux, qaz )

felixfbecker · 2017-09-27T02:28:58Z

src/project-manager.ts

@@ -852,10 +864,39 @@ export class ProjectConfiguration {
 			this.logger
 		);
 		this.service = ts.createLanguageService(this.host, this.documentRegistry);
+		const pluginLoader = new PluginLoader(this.rootFilePath, this.fs, this.pluginSettings, this.logger);
+		pluginLoader.loadPlugins(options, this.enableProxy.bind(this) /* (factory, config) => this.enableProxy(factory, config)) */);


don't use bind(), it's not type safe. wrap in an arrow function instead

felixfbecker · 2017-09-27T02:30:47Z

src/typescript-service.ts

-}
+export type Settings = {
+	format: ts.FormatCodeSettings
+} & PluginSettings;


prefer interface Settings extends PluginSettings { ... }

Also clean up unused code adding TODOs

Exposes initializationOptions containing globalPlugins, allowLocalPluginLoads, pluginProbeLocations Also removes default config and outdated comments

…angeConfiguration

Also add 'node_modules' to resolvedPath Don't give up when resolveJavaScriptModule fails

felixfbecker · 2017-09-27T18:21:11Z

Good to merge once lint failure is fixed

tomv564 · 2017-09-27T19:28:10Z

Thanks for the work you put into merging yesterday! I'll try to clear up any of the outstanding issues with Typescript and ping you when the branch has received the possible short-term fixes.

tomv564 · 2017-09-29T21:24:36Z

Test is fixed, confirmed it still works on Windows too!
Too bad about the coverage stats, wrapService is untested but it's private and only called by successful plugin imports.

Kronuz · 2018-06-13T15:16:33Z

As some plugins also receive configuration options themselves (configFile, ignoreDefinitionFiles, etc., for tslint-language-service), globalPlugins could be a list of objects, similarly to the tsconfig.json configuration for "compilerOptions" (globalPlugins could even be renamed to simply plugins to make it the same):

"plugins": [
  {
    "name": "tslint-language-service",
    "alwaysShowRuleFailuresAsWarnings": false,
    "ignoreDefinitionFiles": true,
    "configFile": "../tslint.json",
    "disableNoUnusedVariableRule": false,
    "supressWhileTypeErrorsPresent": false,
    "mockTypeScriptVersion": false
  }
]

I'd suggest a way to be able to pass those to the plugin maybe passing the whole object here, prior maybe adding global: true to it:

javascript-typescript-langserver/src/plugins.ts

Line 122 in 54304b8

    
           this.enablePlugin({ name: globalPluginName, global: true } as ts.PluginImport, searchPaths, applyProxy)

What do you think?

felixfbecker reviewed Aug 8, 2017

View reviewed changes

angelozerr mentioned this pull request Aug 11, 2017

Use typescript-language-server instead of using javascript-typescript-langserver eclipse-wildwebdeveloper/wildwebdeveloper#22

Closed

tomv564 force-pushed the feature-ts-plugins branch from b2dff0a to cc4c951 Compare August 29, 2017 06:46

tomv564 force-pushed the feature-ts-plugins branch from cc4c951 to a0229ea Compare September 9, 2017 20:29

felixfbecker mentioned this pull request Sep 13, 2017

Document formatting atom/ide-typescript#16

Closed

tomv564 force-pushed the feature-ts-plugins branch from c94f134 to 21ac43e Compare September 16, 2017 21:35

felixfbecker reviewed Sep 20, 2017

View reviewed changes

felixfbecker reviewed Sep 27, 2017

View reviewed changes

tomv564 and others added 9 commits September 27, 2017 07:48

feat: hackish implementation of plugin loading for tslint

9b9c821

fix: Improve plugin import logic

112102d

Also clean up unused code adding TODOs

fix: don't override diagnostic source if provided

77429ce

fix: Add documentation and reorder functions

3df438c

fix: plugin loading for windows

64d7378

feat: plugin configuration

0d79881

Exposes initializationOptions containing globalPlugins, allowLocalPluginLoads, pluginProbeLocations Also removes default config and outdated comments

fix: missing semicolon

786b0c0

fix: extract plugin functionality + add tests

0f0f039

fix: Fix and clean up tests

2292e96

tomv564 added 3 commits September 27, 2017 07:48

fix: use arrow fun, log error, state plugin source

7382e8f

fix: Rename didChangeConfiguration handler to expected workspaceDidCh…

791074d

…angeConfiguration

fix: Switch to didChangeConfiguration, always use local filesystem

d807ec3

Also add 'node_modules' to resolvedPath Don't give up when resolveJavaScriptModule fails

tomv564 force-pushed the feature-ts-plugins branch from e517506 to d807ec3 Compare September 27, 2017 05:49

fix: Review fixes: rename enableProxy, extend PluginSettings, formatting

d3e4419

felixfbecker approved these changes Sep 27, 2017

View reviewed changes

tomv564 and others added 3 commits September 29, 2017 08:23

fix: missing semicolon

bbfd5b4

Merge branch 'master' into feature-ts-plugins

e333011

fix: paths for windows test

42ce6e8

felixfbecker approved these changes Sep 29, 2017

View reviewed changes

felixfbecker merged commit 9cd02d0 into sourcegraph:master Sep 29, 2017

tomv564 deleted the feature-ts-plugins branch September 30, 2017 07:25

Support TS plugins #327

Support TS plugins #327

Uh oh!

Conversation

tomv564 commented Aug 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

felixfbecker commented Aug 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomv564 commented Sep 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomv564 commented Sep 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomv564 Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixfbecker commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixfbecker commented Sep 27, 2017

Uh oh!

tomv564 commented Sep 27, 2017

Uh oh!

tomv564 commented Sep 29, 2017

Uh oh!

Kronuz commented Jun 13, 2018

Uh oh!

Uh oh!

tomv564 commented Aug 6, 2017 •

edited

Loading

codecov bot commented Aug 6, 2017 •

edited

Loading

tomv564 commented Sep 10, 2017 •

edited

Loading

tomv564 Sep 25, 2017 •

edited

Loading